feat: reward aggregator #875
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces significant enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
types/backTypes.d.ts (4)
494-502: LGTM! Consider consistent formatting for improved readability.The changes to
CreateCustomApitype look good. The addition of optionalapi_urlandregexproperties provides more flexibility for API creation. The formatting changes improve readability.For consistency, consider adding a trailing comma after the last property:
export type CreateCustomApi = { quest_id: number; name: string; desc: string; href: string; cta: string; api_url?: string; regex?: string; };This style is often preferred in TypeScript as it makes future additions easier and produces cleaner diffs.
504-512: LGTM! Consider consistent formatting for improved readability.The changes to
UpdateCustomApitype look good. The formatting improvements and the addition of optional properties are consistent with the changes made toCreateCustomApi.For consistency, consider adding a trailing comma after the last property:
export type UpdateCustomApi = { id: number; name?: string; desc?: string; href?: string; cta?: string; api_url?: string; regex?: string; };This style is often preferred in TypeScript as it makes future additions easier and produces cleaner diffs.
514-517: LGTM! Consider adding a clarifying comment foramount.The new
Rewardtype looks good and appropriately represents a reward with an amount and a token symbol.Consider adding a comment to explain why
amountis a string instead of a number. This could help prevent confusion for other developers:export type Reward = { // amount is a string to preserve precision for large or small numbers amount: string; token_symbol: string; };This clarification can be particularly helpful in cryptocurrency contexts where precise representation of values is crucial.
519-530: LGTM! Consider using a more flexible structure forRewardsPerProtocol.Both the
RewardsPerProtocolandCalltypes look good and are well-structured for their purposes. TheCalltype, in particular, aligns well with the PR objective of implementing a feature to aggregate rewards.For
RewardsPerProtocol, consider using a more flexible structure that allows for easy addition of new protocols in the future:export type RewardsPerProtocol = { [protocol: string]: Reward[]; };This change would make the type more maintainable as it wouldn't require modification every time a new protocol is added. It also aligns well with the dynamic nature of the DeFi ecosystem.
The
Calltype is well-defined and appropriate for representing contract calls.services/apiService.ts (1)
416-434: LGTM: NewgetRewardsfunction implemented correctly with room for improvements.The
getRewardsfunction is well-implemented and aligns with the PR objective of aggregating rewards for users. It correctly fetches and processes the rewards data. However, consider the following suggestions for improvement:
- Add input validation for the
addressparameter.- Handle potential empty or invalid responses from the API.
- Consider using TypeScript type annotations for the function parameters and return type to enhance type safety.
Here's a suggested implementation with these improvements:
export const getRewards = async (address: string): Promise<{ rewards: any, calls: { contractAddress: string, entrypoint: string, calldata: string[] }[] } | null> => { if (!address || typeof address !== 'string') { console.error('Invalid address provided'); return null; } try { const response = await fetch(`${baseurl}/defi/rewards?addr=${address}`); if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } const data = await response.json(); if (!data || !data.rewards || !Array.isArray(data.calls)) { console.error('Invalid response structure'); return null; } const rewards = data.rewards; const parsedCalls = data.calls.map((call: Call) => ({ contractAddress: call.contractaddress, entrypoint: call.entrypoint, calldata: call.calldata, })); return { rewards, calls: parsedCalls }; } catch (err) { console.error("Error while fetching rewards", err); return null; } };This implementation includes input validation, better error handling, and type annotations. It also uses
console.errorfor logging errors, which is more appropriate for error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
public/nostra/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (4)
- components/discover/claimModal.tsx (4 hunks)
- services/apiService.ts (2 hunks)
- types/backTypes.d.ts (2 hunks)
- types/frontTypes.d.ts (2 hunks)
🧰 Additional context used
🪛 Biome
components/discover/claimModal.tsx
[error] 188-188: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (5)
types/frontTypes.d.ts (2)
345-349: LGTM: NewCalltype aligns with PR objectivesThe new
Calltype is well-defined and follows TypeScript best practices. It provides a clear structure for representing contract calls, which is crucial for implementing the reward aggregation feature. This type will likely be used in the process of claiming rewards across different protocols, supporting the main objective of the PR.
320-320: LGTM: Improved formatting forStepMaptypeThe formatting change to the last union member of the
StepMaptype improves consistency and readability. While this change doesn't directly relate to the PR objectives, it contributes to overall code quality.types/backTypes.d.ts (1)
Line range hint
1-530: Overall, the changes look good and align well with the PR objectives.The modifications and additions to the type definitions in
types/backTypes.d.tsprovide a solid foundation for implementing the reward aggregation feature. The new types (Reward,RewardsPerProtocol, andCall) directly support this objective, while the updates to existing types (CreateCustomApiandUpdateCustomApi) improve flexibility and readability.The suggestions provided in the review are minor and focus on improving maintainability and clarity. These include:
- Using more specific types where possible (e.g., for
callsinCreateContract)- Consistent formatting (adding trailing commas)
- Adding clarifying comments
- Using a more flexible structure for
RewardsPerProtocolImplementing these suggestions would further enhance the robustness and maintainability of the codebase. Great work on laying the groundwork for the reward aggregation feature!
services/apiService.ts (2)
28-28: LGTM: Import statement updated correctly.The addition of the
Calltype to the import statement is appropriate and consistent with its usage in the newgetRewardsfunction. This change enhances type safety for thecallobjects processed in the function.
Line range hint
1-434: Summary: Changes align well with PR objectives, with minor suggestions for improvement.The additions to
services/apiService.tseffectively implement the reward aggregation feature described in the PR objectives. The newgetRewardsfunction provides a mechanism to fetch and process user rewards, which is crucial for the "Claim all" functionality mentioned in the linked issue #858.The implementation is generally sound, with consistent error handling and appropriate data processing. The suggestions provided for improving input validation, error handling, and type safety in the
getRewardsfunction will further enhance the robustness of this feature.These changes contribute significantly to the goal of streamlining the reward claiming process for users across various Starknet protocols, as outlined in the PR objectives.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/discover/claimModal.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (7)
components/discover/claimModal.tsx (7)
13-16: LGTM: New imports for Starknet integrationThe added imports for Starknet-related hooks, types, and utilities are appropriate for the new functionality of fetching and managing rewards.
19-21: LGTM: Updated type definitionsThe changes to
RewardItemandCurrencyRowPropstypes, using semicolons for property separation, align with TypeScript conventions and accurately reflect the data structure used in the component.Also applies to: 26-26
34-63: LGTM: Improved RewardComponent structure and stylingThe
RewardComponenthas been updated with better semantic HTML and CSS classes, improving its structure and styling. The use of theAppIconcomponent with custom styling enhances the visual representation of each reward item.
66-76: LGTM: Enhanced CurrencyRow componentThe
CurrencyRowcomponent has been updated to use the newCurrencyRowPropstype and now includes formatting for large currency values (>= 1000) using the 'K' suffix. This improves readability and follows React best practices.
86-91: LGTM: Updated ClaimModal state and Starknet hooksThe addition of
rewardsandcallsstate variables, along with the use ofuseAccountanduseContractWritehooks from Starknet, provides the necessary functionality for managing fetched rewards data and interacting with the blockchain.
Line range hint
1-199: Overall assessment: Well-implemented reward aggregation featureThe changes in this file successfully implement the reward aggregation feature, aligning well with the PR objectives. Key improvements include:
- Integration with Starknet for blockchain interactions.
- Dynamic rendering of rewards from multiple protocols.
- Updated UI components for better user experience.
The code is generally well-structured and follows React and TypeScript best practices. A few minor suggestions have been made to enhance error handling and simplify the rendering logic.
Great job on implementing this feature! Once the suggested improvements are addressed, this implementation will provide a robust solution for users to view and claim their rewards across multiple Starknet protocols.
93-100:⚠️ Potential issueAdd error handling for getRewards API call
While the useEffect hook correctly fetches rewards when the address is available, it lacks error handling. This could lead to unhandled exceptions if the API call fails.
Consider adding a try-catch block or using the .catch() method to handle potential errors:
useEffect(() => { if (!address) return; getRewards(address) .then((res) => { setRewards(res?.rewards); setCalls(res?.calls); setLoading(false); }) + .catch((error) => { + console.error("Error fetching rewards:", error); + showNotification("Failed to fetch rewards", "error"); + setLoading(false); + }); }, [address]);This ensures that any API errors are logged, the user is notified, and the loading state is properly managed.
Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
components/discover/claimModal.tsx (1)
Line range hint
124-173: Fix mismatched JSX tags to resolve parsing errorsThere are missing closing tags for the
<div>and<Loading>components, leading to parsing errors reported by the static analysis tools.Ensure that every opening tag has a corresponding closing tag. Here's how you can fix it:
Add the closing tag for the
<div>opened at line 124 and the<Loading>component opened at line 127:...
- Line 171: Close the
<Loading>component.- Line 172: Close the
<div>opened at line 124.This ensures proper nesting of JSX elements and resolves the parsing errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/discover/claimModal.tsx (4 hunks)
🧰 Additional context used
🪛 Biome
components/discover/claimModal.tsx
[error] 127-127: Expected corresponding JSX closing tag for 'Loading'.
Opening tag
closing tag
(parse)
[error] 124-126: Expected corresponding JSX closing tag for 'div'.
Opening tag
closing tag
(parse)
There was a problem hiding this comment.
Test E2E
- When no rewards I have no message, I think you can check for rewards when arriving on the page and make the button available only if rewards are to be claimed
- Infinite loading when no wallet is connected, I think you can juste delete the option to click on the button if there is address
fricoben
added
❌ Change request
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
components/discover/claimModal.tsx (2)
34-75: Improved component definitions with better type safetyThe
RewardComponentandCurrencyRowhave been updated to use theFunctionComponenttype and the revised type definitions. This enhances type safety and improves the overall structure of the components.One minor suggestion:
Consider using the more concise arrow function syntax for
CurrencyRow:const CurrencyRow: FunctionComponent<CurrencyRowProps> = ({ currencyName, currencyValue, }) => ( // ... component body ... );This change would make the syntax consistent with the
RewardComponent.
Line range hint
79-193: Overall implementation successfully achieves PR objectivesThe updated
ClaimModalcomponent effectively implements the reward aggregator feature, aligning well with the PR objectives. Key achievements include:
- Integration with Starknet using appropriate hooks.
- Fetching and displaying rewards from multiple protocols.
- Implementing a "Claim all" functionality.
These changes significantly enhance the user experience by allowing users to view and claim all their rewards in one action, as outlined in the linked issue #858.
While the core functionality is well-implemented, consider addressing the following points to further improve the component:
- Enhance error handling in the
useEffecthook for reward fetching.- Remove sensitive data from error logging in the
doClaimRewardsfunction.- Optimize the reward rendering logic for better performance and readability.
- Ensure proper loading state management throughout the component.
To further improve the component's modularity and maintainability, consider:
- Extracting the reward fetching logic into a custom hook.
- Creating a separate utility function for formatting reward values.
- Implementing a more robust error handling strategy throughout the component.
These architectural improvements will make the code more maintainable and easier to test in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/discover/claimModal.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
components/discover/claimModal.tsx (2)
13-16: New imports enhance functionality and type safetyThe addition of these imports indicates a shift towards more robust blockchain integration:
useAccountanduseContractWritefrom@starknet-react/coresuggest improved Starknet integration.RewardsPerProtocoltype import enhances type safety.getRewardsandgweiToEthimports indicate the implementation of reward fetching and conversion logic.These changes align well with the PR objective of implementing a reward aggregator feature.
19-21: Improved type definitions enhance code clarityThe updates to
RewardItemandCurrencyRowPropstypes, including the use of semicolons for property separation, improve code consistency and readability. This change aligns with TypeScript best practices.Also applies to: 26-26
Marchand-Nicolas
added
🔥 Ready for review
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
components/discover/defiTable.tsx (2)
330-330: LGTM with suggestion: Consider adding user feedback for disconnected state.The addition of the address check before showing the claim modal is a good practice. It prevents the modal from appearing when the user is not connected.
However, consider providing visual feedback or a tooltip for users who aren't connected. This could improve the user experience by clearly indicating why the button might not be clickable.
Would you like me to suggest an implementation for this feedback mechanism?
Line range hint
1-504: Overall assessment: Implementation aligns with PR objectives, with minor suggestions for improvement.The changes successfully implement the core functionality for aggregating rewards, as outlined in the PR objectives. The integration of the user's account address and the updated "Claim all" button behavior contribute to a more robust user experience.
To further enhance the implementation, consider the following suggestions:
- Implement visual feedback for users who aren't connected when they attempt to use the "Claim all" button.
- Add error handling for scenarios where the account address might be undefined or null.
- Consider adding a loading state for the "Claim all" button while the modal is being prepared to open.
These improvements would further enhance the user experience and make the feature more resilient.
Would you like assistance in implementing any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/discover/claimModal.tsx (4 hunks)
- components/discover/defiTable.tsx (8 hunks)
🧰 Additional context used
🔇 Additional comments (6)
components/discover/defiTable.tsx (6)
39-39: LGTM: Import statement for useAccount added correctly.The import of
useAccountfrom@starknet-react/coreis appropriately placed and will allow access to the user's account information, which is essential for the new functionality being implemented.
251-252: LGTM: useAccount hook implemented correctly.The
useAccounthook is properly used to extract the user's address. This implementation aligns with the PR objective of enhancing user-specific functionality for claiming rewards.
331-332: Minor formatting changes noted.The adjustments to the className are minor and appear to be for code style consistency. These changes don't affect functionality and are acceptable.
423-430: Minor formatting changes noted in table header onClick handler.The adjustments to the onClick handler and the flexRender function call are minor and appear to be for code style consistency. These changes don't affect functionality and are acceptable.
445-450: Minor formatting changes noted in table row onClick handler.The adjustments to the onClick handler for the table row are minor and appear to be for code style consistency. These changes don't affect functionality and are acceptable.
476-478: LGTM: Improved pagination control styles.The updates to the style props for the pagination controls enhance the user experience by providing clear visual feedback when pagination is not possible. This is a good improvement in terms of usability.
Also applies to: 490-490
| setLoading(false); | ||
| closeModal(); | ||
| showSuccess(); | ||
| } catch (error) { | ||
| setLoading(false); | ||
| showNotification("Error while claiming rewards", "error"); | ||
| console.log("Error while claiming rewards", error); | ||
| console.log("Error while claiming rewards", error, calls); |
There was a problem hiding this comment.
Avoid logging sensitive data in the console
Including calls in the console log may expose sensitive information. It's recommended to log only the error.
Apply this diff to remove calls from the log statement:
- console.log("Error while claiming rewards", error, calls);
+ console.log("Error while claiming rewards", error);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("Error while claiming rewards", error, calls); | |
| console.log("Error while claiming rewards", error); |
| getClaimRewards(); | ||
| } | ||
| }, [open, getClaimRewards]); | ||
| }, [calls]); |
There was a problem hiding this comment.
Update dependency array to include all external dependencies
The dependency array for doClaimRewards is missing some external variables used within the function. This may lead to stale closures if those variables change.
Apply this diff to include all necessary dependencies:
- }, [calls]);
+ }, [calls, execute, closeModal, showSuccess]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, [calls]); | |
| }, [calls, execute, closeModal, showSuccess]); |
| useEffect(() => { | ||
| if (!address) return; | ||
| getRewards(address) | ||
| .then((res) => { | ||
| setRewards(res?.rewards); | ||
| setCalls(res?.calls); | ||
| setLoading(false); | ||
| }) | ||
| .catch((error) => { | ||
| showNotification("Error while fetching rewards", "error"); | ||
| console.log("Error while fetching rewards", error); | ||
| }); | ||
| }, [address]); |
There was a problem hiding this comment.
Ensure loading state is properly managed in error scenarios
In the useEffect hook, if an error occurs while fetching rewards, the setLoading(false) function is not called. This may cause the loading spinner to remain indefinitely.
Apply this diff to set the loading state in the catch block:
.catch((error) => {
showNotification("Error while fetching rewards", "error");
console.log("Error while fetching rewards", error);
+ setLoading(false);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!address) return; | |
| getRewards(address) | |
| .then((res) => { | |
| setRewards(res?.rewards); | |
| setCalls(res?.calls); | |
| setLoading(false); | |
| }) | |
| .catch((error) => { | |
| showNotification("Error while fetching rewards", "error"); | |
| console.log("Error while fetching rewards", error); | |
| }); | |
| }, [address]); | |
| useEffect(() => { | |
| if (!address) return; | |
| getRewards(address) | |
| .then((res) => { | |
| setRewards(res?.rewards); | |
| setCalls(res?.calls); | |
| setLoading(false); | |
| }) | |
| .catch((error) => { | |
| showNotification("Error while fetching rewards", "error"); | |
| console.log("Error while fetching rewards", error); | |
| setLoading(false); | |
| }); | |
| }, [address]); |
close: #858
Summary by CodeRabbit
New Features
ClaimModalcomponent for improved blockchain interaction and responsiveness.defiTablecomponent to manage user account information effectively.Bug Fixes
Documentation
Chores